Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

move version map register to begin block #44

Merged
merged 3 commits into from
Jan 18, 2023
Merged

move version map register to begin block #44

merged 3 commits into from
Jan 18, 2023

Conversation

nghuyenthevinh2000
Copy link
Contributor

Summary of changes

Move version map to BeginBlocker

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated API documentation (client/lcd/swagger-ui/swagger.yaml)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

@inon-man
Copy link
Collaborator

inon-man commented Jan 14, 2023 •
edited
Loading

Why does the replacement required? It's weird to me that setting version map is required at every new block. Which is different from https://github.com/cosmos/cosmos-sdk/blob/5213bbd65326edb9cd84f70c1bc0232b7389a4f7/simapp/app.go#L580

IMO, There should be better solution if it's due to some problem.

@ZaradarBH
Copy link
Contributor

I agree with @inon-man. This doesnt seem right. I think we should all get together and talk about the upgrade handlers before we get to fare ahead of ourselves on the subject. :)

@nghuyenthevinh2000
Copy link
Contributor Author

after Edward shows me that InitChain() will not be reached if we continue the chain and not from block zero. I kinda suspect whether continuing from 1.0.4 to 1.0.5 will reach InitChainer(). If so, the version map registration will never be reached if it is in InitChainer()

Also from Tendermint here, https://github.com/tendermint/tendermint/blob/main/consensus/replay.go#L303, RequestInitChain only happens if appBlockHeight == 0. abci.RequestInitChain{} request will invoke InitChainer().

For appBlockHeight > 0, abci.RequestInitChain{} request will not be sent.

@inon-man
Copy link
Collaborator

@nghuyenthevinh2000 I see. It seems that we only need to do it once in the hotfix.
I propose to keep the old SetModuleVersionMap in InitChainer to be consistent with the official cosmos-sdk method and remove BeginBlocker patch in the next upgrade.

ZaradarBH
ZaradarBH reviewed Jan 16, 2023
View reviewed changes
app/app.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 16, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (v1.0.5-upgrade@7fe4468). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##             v1.0.5-upgrade      #44   +/-   ##
=================================================
  Coverage                  ?   64.28%           
=================================================
  Files                     ?      100           
  Lines                     ?     4645           
  Branches                  ?        0           
=================================================
  Hits                      ?     2986           
  Misses                    ?     1444           
  Partials                  ?      215

@nghuyenthevinh2000
Copy link
Contributor Author

per @ZaradarBH suggest, we should check if Upgrade handler's version map is empty in mainnet:

  • a full node
  • modified binary version 1.0.4 with version map logging should do

@inon-man
Copy link
Collaborator

inon-man commented Jan 17, 2023 •
edited
Loading

I will check len(vm) == 0 on my node by logging it.

@inon-man
Copy link
Collaborator

inon-man commented Jan 17, 2023 •
edited
Loading

I spinned up full node (v1.0.4) with pruned columbus-5 data and it has empty version map.

node-terrad-1  | 2:23PM INF indexed block height=11116014 module=txindex
node-terrad-1  | 2023/01/17 14:23:58 ------------- VM LEN 0 -----------
node-terrad-1  | 2:24PM INF executed block height=11116015 module=state num_invalid_txs=0 num_valid_txs=123
node-terrad-1  | 2:24PM INF commit synced commit=436F6D6D697449447B5B39312031313220333520313931203137362037342031373520353720323437203138322032372032343920313533203134322031303120313133203137342031363020373820343920313432203131372032323320313030203232332032343120313834203234312031373820313931203139203133335D3A4139394445467D
node-terrad-1  | 2:24PM INF committed state app_hash=5B7023BFB04AAF39F7B61BF9998E6571AEA04E318E75DF64DFF1B8F1B2BF1385 height=11116015 module=state num_txs=123
node-terrad-1  | 2:24PM INF indexed block height=11116015 module=txindex
node-terrad-1  | 2023/01/17 14:24:00 ------------- VM LEN 0 -----------
node-terrad-1  | 2:24PM INF client state updated client-id=07-tendermint-12 height=1-7823674 module=x/ibc/client
node-terrad-1  | Dragonberry Active
node-terrad-1  | 2:24PM INF packet received module=x/ibc/channel packet="{477285 transfer channel-72 transfer channel-1 [123 34 97 109 111 117 110 116 34 58 34 49 51 53 54 48 48 52 56 55 54 57 55 34 44 34 100 101 110 111 109 34 58 34 116 114 97 110 115 102 101 114 47 99 104 97 110 110 101 108 45 55 50 47 117 108 117 110 97 34 44 34 114 101 99 101 105 118 101 114 34 58 34 116 101 114 114 97 49 102 117 114 55 55 109 97 54 50 55 57 121 109 104 101 97 113 122 118 107 103 119 117 97 109 110 109 116 114 97 110 109 57 117 120 100 109 107 34 44 34 115 101 110 100 101 114 34 58 34 111 115 109 111 49 51 108 117 114 104 48 116 57 119 55 116 114 109 101 101 122 97 56 55 54 97 117 51 120 122 103 100 102 107 118 116 119 57 112 102 110 54 100 34 125] 5-11116154 0}"
node-terrad-1  | 2:24PM INF acknowledged written module=x/ibc/channel packet="{477285 transfer channel-72 transfer channel-1 [123 34 97 109 111 117 110 116 34 58 34 49 51 53 54 48 48 52 56 55 54 57 55 34 44 34 100 101 110 111 109 34 58 34 116 114 97 110 115 102 101 114 47 99 104 97 110 110 101 108 45 55 50 47 117 108 117 110 97 34 44 34 114 101 99 101 105 118 101 114 34 58 34 116 101 114 114 97 49 102 117 114 55 55 109 97 54 50 55 57 121 109 104 101 97 113 122 118 107 103 119 117 97 109 110 109 116 114 97 110 109 57 117 120 100 109 107 34 44 34 115 101 110 100 101 114 34 58 34 111 115 109 111 49 51 108 117 114 104 48 116 57 119 55 116 114 109 101 101 122 97 56 55 54 97 117 51 120 122 103 100 102 107 118 116 119 57 112 102 110 54 100 34 125] 5-11116154 0}"
node-terrad-1  | 2:24PM INF executed block height=11116016 module=state num_invalid_txs=0 num_valid_txs=3
node-terrad-1  | 2:24PM INF commit synced commit=436F6D6D697449447B5B383520313332203232332031363220313634203230342032333620323520323039203230302031313820323133203131322032323220313732203437203534203132203735203232392031343020333320383820313336203634203733203139332035203337203431203431203132355D3A4139394446307D
node-terrad-1  | 2:24PM INF committed state app_hash=5584DFA2A4CCEC19D1C876D570DEAC2F360C4BE58C2158884049C1052529297D height=11116016 module=state num_txs=3
node-terrad-1  | 2:24PM INF indexed block height=11116016 module=txindex
node-terrad-1  | 2023/01/17 14:24:03 ------------- VM LEN 0 -----------
node-terrad-1  | 2:24PM INF executed block height=11116017 module=state num_invalid_txs=0 num_valid_txs=0

@edk208 edk208 requested review from inon-man and ZaradarBH January 17, 2023 20:54
@edk208
Copy link
Contributor

edk208 commented Jan 17, 2023 •
edited
Loading

added block height guards, to trigger Feb 14th
all nodes will have this set at this block height, regardless of it being set or not ahead of time

@ZaradarBH ZaradarBH added enhancement New feature or request in scope Work approved by the community labels Jan 18, 2023
@ZaradarBH ZaradarBH added this to the v1.0.5 milestone Jan 18, 2023
@ZaradarBH ZaradarBH merged commit 8bb56e9 into v1.0.5-upgrade Jan 18, 2023
@inon-man inon-man deleted the v1.0.5-vm-fix branch February 7, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZaradarBH ZaradarBH ZaradarBH approved these changes

@edk208 edk208 edk208 approved these changes

@faddat faddat Awaiting requested review from faddat

@fragwuerdig fragwuerdig Awaiting requested review from fragwuerdig

@inon-man inon-man Awaiting requested review from inon-man

Assignees

@nghuyenthevinh2000 nghuyenthevinh2000

Labels
enhancement New feature or request in scope Work approved by the community
Projects
No open projects
L1 Sprint #1 Jan 9th - Jan 20th
Status: ✅ Done
Milestone
v1.0.5
Development

Successfully merging this pull request may close these issues.
4 participants
@nghuyenthevinh2000 @inon-man @ZaradarBH @edk208